Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added 2 tests - a true and a false positive #5529

Closed
wants to merge 1 commit into from
Closed

added 2 tests - a true and a false positive #5529

wants to merge 1 commit into from

Conversation

brah-mcdude
Copy link
Contributor

@brah-mcdude brah-mcdude commented Jan 23, 2022 •
edited
Loading

Hi @Aaronontheweb.
[DO NOT MERGE - This is not designed to be an actual pull request - this is just a question - but a very important one - please read]
I have been sitting on this for a couple of days. So I am turning to you as a last resort.
I am unable to understand an issue that I have been having.
I added 2 tests to a sample test.

  1. ReplyingToProbeMessagesFunc
  2. ReplyingToProbeMessagesAction

Notice that 1 passes and 2 fails.
But if you debug 1, you will see that the test should fail.
As the condition asserted is false.
So while debugging, an assertion pops up.
But if you continue debugging, the TEST PASSES.
What is going on?
This is something I find difficult to live with,
as this seems to only occur with EventFilter's ExpectAsync actionAsync
which I depend on in all my tests.
Which means that I cannot use the await keyword inside my test code:(
Is it a bug in Visual Studio?
In xUnit?
In Akka?
Or is it my bad?
Either way, we need to at the very least be aware of whatever it is.
Thank you.

@Aaronontheweb
Copy link
Member

I'll try to get you an answer on this today or tomorrow

@Aaronontheweb
Copy link
Member

So I see that DocsExamples.Testkit.ProbeSampleTest.ReplyingToProbeMessagesAction fails consistently with:

Akka.TestKit.Xunit2.Internals.AkkaEqualException : Assert.Equal() Failure
Expected: world2
Actual:   world

This fails because

await EventFilter.Error().ExpectAsync(0, action: () =>
  {
      var probe = CreateTestProbe();
      probe.Tell("hello");
      probe.ExpectMsg("hello");
      probe.Reply("world");
      ExpectMsg("world2");
      Assert.Equal(probe.Ref, LastSender);
  });

We reply with world and not world2 - you probably figured that one out already.

The biggest mystery is why does this pass:

/// <summary>
/// this test is a false positive
/// </summary>
/// <returns></returns>
[Fact]
public async Task ReplyingToProbeMessagesFunc()
{
    await EventFilter.Error().ExpectAsync(0, actionAsync: async () =>
    {
        var probe = CreateTestProbe();
        probe.Tell("hello");
        probe.ExpectMsg("hello");
        probe.Reply("world");
        await Task.Run(() => { ExpectMsg("world2"); });
        Assert.Equal(probe.Ref, LastSender);
    });
}

Looks to me like we forgot to.... you know, await the Task returned inside here:

akka.net/src/core/Akka.TestKit/EventFilter/Internal/EventFilterApplier.cs

Lines 454 to 460 in 38b7443

/// <summary>
/// Async version of <see cref="InternalExpect"/>
/// </summary>
private async Task InternalExpectAsync(Func<Task> actionAsync, ActorSystem actorSystem, int expectedCount, TimeSpan? timeout = null)
{
await InterceptAsync<object>(() => { actionAsync(); return Task.FromResult<object>(null); }, actorSystem, timeout, expectedCount);
}

So I suspect the await Task.Run(() => { ExpectMsg("world2"); }); is running as a detatched task, which is incredibly annoying. We just need to update the TestKit to await those tasks internally and then this should pass.

@Aaronontheweb
Copy link
Member

Does that answer your question @brah-mcdude ?

@Aaronontheweb
Copy link
Member

This is definitely a bug, we should patch it, and we can use your spec as a reproduction. Akka.TestKit.Tests is a good place to add this.

@brah-mcdude
Copy link
Contributor Author

Would you like me to do it?

@Aaronontheweb
Copy link
Member

Would you like me to do it?

That would be great

@brah-mcdude
Copy link
Contributor Author

On it:)

@brah-mcdude brah-mcdude mentioned this pull request Jan 25, 2022
Closed
@brah-mcdude brah-mcdude closed this Feb 3, 2022
@brah-mcdude brah-mcdude deleted the ExpectAsyncFalsePositive branch February 3, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brah-mcdude @Aaronontheweb